Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement tooling to generate multiple plotly schemas #18

Merged

Conversation

PatrickVienne
Copy link
Contributor

No description provided.

Copy link
Owner

@MetalBlueberry MetalBlueberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the contribution!! ❤️

I've left a few comments with some questions 😄

generator/typefile.go Outdated Show resolved Hide resolved
generator/cmd/generator/main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
generator/renderer.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@PatrickVienne
Copy link
Contributor Author

@MetalBlueberry
thanks for the comments, back to you.

@MetalBlueberry
Copy link
Owner

MetalBlueberry commented Apr 14, 2024

Okey, that last commit is an attempt to reduce the number of changes on this PR.

I don't think we have to update go.mod version as the code doesn't require any new feature. It is good to leave it as is and only update dependencies if we identify a vulnerability or we need some functionality.

The idea is that this will allow more people to use the library without worring about updating things.

Also, In the readme you add instructions to bump to a specific version, Do you think we could write that in a way that just instructs people how to update to ANY version? The process should be the same.

Last but not least, I tried to generate the code from this branch and I noticed the area_gen.go file is not generated. I couldn't investigate why it happen nor if it is expected, it just looks odd as I would expect all the previous types to remain valid. I will look into this once I find time for the project.

Thank you for the contribution!!

@PatrickVienne
Copy link
Contributor Author

PatrickVienne commented Apr 20, 2024

Hi @MetalBlueberry

it is not generated, because it is not part of the json schema:

  • go.mod
    -> Agreed

  • add instructioons:
    ->

These are the new trace types:
jq '.schema.traces | keys' schema.json --sort-keys

[
  "bar",
  "barpolar",
  "box",
  "candlestick",
  "carpet",
  "choropleth",
  "choroplethmapbox",
  "cone",
  "contour",
  "contourcarpet",
  "densitymapbox",
  "funnel",
  "funnelarea",
  "heatmap",
  "heatmapgl",
  "histogram",
  "histogram2d",
  "histogram2dcontour",
  "icicle",
  "image",
  "indicator",
  "isosurface",
  "mesh3d",
  "ohlc",
  "parcats",
  "parcoords",
  "pie",
  "pointcloud",
  "sankey",
  "scatter",
  "scatter3d",
  "scattercarpet",
  "scattergeo",
  "scattergl",
  "scattermapbox",
  "scatterpolar",
  "scatterpolargl",
  "scattersmith",
  "scatterternary",
  "splom",
  "streamtube",
  "sunburst",
  "surface",
  "table",
  "treemap",
  "violin",
  "volume",
  "waterfall"
]

Or via jsonviewer:
https://jsonviewer.stack.hu/
image

If this update goes through, I'll submit the next update to the latest version, watching out for your current comments. I also plan to add dark theme, as in plotly for python

I added a downloader script a description how to generate the new files without manual work, only using the scripts.
Also added some more hints in the readme on using the scripts and tools like jq and jsonviewer.

Hope this helps to get it through the finish line :)
Thanks!

@MetalBlueberry
Copy link
Owner

Alright! I added a few steps to the CI to automatically check that the scheme is correctly built and I've identified some bugs with consistency on the scheme generation 😅

Not perfect yet, But I've merged those changes here so we can get better feedback on your changes.

@PatrickVienne
Copy link
Contributor Author

Thanks a lot @MetalBlueberry
anything I can do in support?

@MetalBlueberry
Copy link
Owner

MetalBlueberry commented May 1, 2024

This PR is making me think about how can we support multipe plotly versions? It is true that generating a new schema is straight forward, but it makes it hard for people just wanting to import the package an use it.

One idea that crossed my mind, given that you already wrote a script to download the schema from plotly site, we cloud write a script that downloads multiple schemas to a directory in this repository and then pair it with the generate script so we can generate all the schemas at once.

The proposal is to have a file strucuture that looks like

  • generated/2.29/graph_objects
  • generated/2.18/graph_objects
  • and so on...

This way, any user can import the latest version of this library but still use any plotly version of their choice.

I don't think we have to support all versions, for a first approach I'll sugest to just support 2.29 and 2.18. So the task you can do is.

Task

Write a script that reads a file in the root of the repository with a list of versions that download schemas to a directory called schemas/2.29.0.json and then have another script that generates the respective go schema on a directory generated/2.29/graph_objects

The file will be schemas.yaml and will contain a key versions with a list of object with the necessary information. For example

versions:
  -  Name: Plotly 2.29.0
     URL: https://whatever
     Path: schemas/2.29.0.json
     Generated: generated/2.29/graph_objects

Design decissions

  • The path ends with graph_objects so all versions can be imported with the same name. This makes it very easy to switch between compatible versions.
  • The Schema Path and the Generate Path are part of the struct so we can update schemas but keep the same output directory. useful to just update patch versions.
  • The file with the list of versions allows to decide which versions to support and increase the values as we need it.
  • Keeping the schema on this repository is needed so we don't have to rely on any external service during CI. The download step must be executed by an human and added to the repository with a PR
  • Generation step must be executed on every commit on the CI

Keep in mind this is a suggestion, feel free to sugest any improvements or make changes.
But my advice is to keep it as simple as possible.

This will remove the need to add instructions to the readme about how to run the generator and simply it to Just add the schema to the right directory

@PatrickVienne
Copy link
Contributor Author

@MetalBlueberry

great idea. Just slightly adjusted the yaml.

Hope this captures the essence of what you meant

running the downloader will download all the schema files as defined in the yaml.

running the generator still needs to be done for each version one by one, but can be done based on the downloaded files in the folder.

image

slightly changed the schema yaml to make the versions more in line with the git tags of the plotly repo
image

won't have time for the next weeks to finish it unfortunately, but maybe this can get it started.

open todos still:

  • adjust the readme file
  • also create the plot.go file for separate versions if needed
  • integrate CI generation of newly added versions
  • read the yaml also in the generator, and use the output path of schemas.json to determine the output path.

@PatrickVienne PatrickVienne force-pushed the feature/plotly_v2.29.0 branch 2 times, most recently from 53c1fe9 to ee89afd Compare May 11, 2024 11:08
@PatrickVienne
Copy link
Contributor Author

@MetalBlueberry
completed all the points, and the generation now is also running in the workflow which you setup for testing

Also updated the readme now and added some release notes.

### Add version v2.29.1 and v2.31.1
- Added Downloader for schema file
- added generator for different packages based on plotly version in subfolders for each version
- enable easier use for different plotly version by new import paths which include the version generated. Example: `grob "github.com/MetalBlueberry/go-plotly/generated/v2.31.1/graph_objects"`
- removed go generate directive from the templates which are no longer necessary
- removed offline package, as each version package needs its own **plot.go**.
- Replaced all `offline` imports in the examples packages by just calling the `grob` package of the specified version
- all html's will now refer to the correct plotly version's cdn in the html's head

We've come a long way, thanks for your comments, really cool stuff, hope we can get this merged. and have next steps implemented in nxt PRs soon.
Animations Support and Dark Theme are on my list.

image

@PatrickVienne
Copy link
Contributor Author

@MetalBlueberry
I can't get the wasm example to run though.

one would also need to make sure to use the correct wasm_exec.js
https://github.com/golang/go/blob/go1.21.0/misc/wasm/wasm_exec.js

but this again is a topic for another PR.

@PatrickVienne
Copy link
Contributor Author

@MetalBlueberry
back to you

generator/templates/plotly.tmpl Outdated Show resolved Hide resolved
generator/cmd/generator/main.go Outdated Show resolved Hide resolved
@PatrickVienne
Copy link
Contributor Author

@MetalBlueberry
frames are removed. back to you

@MetalBlueberry
Copy link
Owner

Alright, I made a few tweaks.

  • I removed the changes to the offline package, you can submitt that on a different PR
  • Removed some information from the read me, as I feel that information doesn't belong there. It should be obvious from looking at the code structure. If you still feel that information is useful, it can be added on a different location as specific documentation.
  • Fixed the offline package to have the right import paths and CDN
  • Fixed other small things

If you are happy, I'm happy with this. We could merge it

generator/renderer.go Outdated Show resolved Hide resolved
@PatrickVienne
Copy link
Contributor Author

Thanks. One small cleanup comment, and then let's merge it.

@MetalBlueberry MetalBlueberry changed the title update to plotly version 2.29.0 Implement tooling to generate multiple plotly schemas May 25, 2024
@MetalBlueberry MetalBlueberry merged commit 505d87c into MetalBlueberry:master May 25, 2024
2 checks passed
@MetalBlueberry
Copy link
Owner

Let's Go!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants